-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proxy protocol: new feature auto-detect proxy protocol #18951
proxy protocol: new feature auto-detect proxy protocol #18951
Conversation
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Hi @kdorosh, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
…-fork into auto_detect_proxy_protocol Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
@@ -434,6 +443,9 @@ ReadOrParseState Filter::readProxyHeader(Network::IoHandle& io_handle) { | |||
if (nread < 1) { | |||
ENVOY_LOG(debug, "failed to read proxy protocol (no bytes read)"); | |||
return ReadOrParseState::Error; | |||
} else if (nread < PROXY_PROTO_V2_HEADER_LEN && config_.get()->detectProxyProtocol()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the client send a partial v1 protocol header, I feel this will skip the process the v1 protocol header. it would be great to add an unitest for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback, addressed in ed4fb54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related #18951 (comment)
// NOTICE: only enable if ALL traffic to the listener comes from a trusted source. | ||
// Defaults to false. If true, attempt to detect proxy protocol if present, and allow | ||
// requests through if proxy protocol is not used on the connection. | ||
bool detect_proxy_protocol = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the major goal is allow the requests through if proxy protocol is not used. And actually the whole proxy protocol filter is about detect the proxy protocol, so this option name feels confuse. Should we call it something like allow_requests_with_proxy_protocol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 please clarify the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better as well, addressed in bcc7aef
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
Left a few comments.
@@ -40,4 +40,9 @@ message ProxyProtocol { | |||
|
|||
// The list of rules to apply to requests. | |||
repeated Rule rules = 1; | |||
|
|||
// NOTICE: only enable if ALL traffic to the listener comes from a trusted source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably in an .. attention::
clause.
See example.
I also suggest to rephrase the comment, starting with with it does, its default value, and then the notice part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example, addressed in 91c9a1b
// NOTICE: only enable if ALL traffic to the listener comes from a trusted source. | ||
// Defaults to false. If true, attempt to detect proxy protocol if present, and allow | ||
// requests through if proxy protocol is not used on the connection. | ||
bool detect_proxy_protocol = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 please clarify the name.
@@ -47,6 +47,7 @@ Config::Config( | |||
Stats::Scope& scope, | |||
const envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol& proto_config) | |||
: stats_{ALL_PROXY_PROTOCOL_STATS(POOL_COUNTER(scope))} { | |||
detect_proxy_protocol_ = proto_config.detect_proxy_protocol(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be set in the c'tor's member initialization list above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c45ecc7
* Filter configuration that determines if we should pass-through failed proxy protocol | ||
* requests. Should only be configured to true for trusted downstreams. | ||
*/ | ||
bool detectProxyProtocol() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here should also be clear (following the api field name change).
@@ -222,6 +222,19 @@ TEST_P(ProxyProtocolTest, V1Basic) { | |||
disconnect(); | |||
} | |||
|
|||
TEST_P(ProxyProtocolTest, DetectNoProxyProtocol) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment on what the test is validating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 4259f1a, please let me know if this doesn't clarify what might have been confusing
// Release the file event so that we do not interfere with the connection read events. | ||
io_handle.resetFileEvents(); | ||
cb_->continueFilterChain(true); | ||
return ReadOrParseState::Done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't need to return anything, and the returned status should probably be only at the end of Filter::onReadWorker()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, addressed in 2b4a89a
Just realized there was a race where some of the comments were addressed in a recent commit. |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
@adisuissa all comments should be addressed |
proto_config.set_allow_requests_without_proxy_protocol(true); | ||
connect(true, &proto_config); | ||
|
||
write("PROXY TCP4"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test the case of the first write is not full v1 protocol signature, like just one char 'P'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 6a0d49b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure this is possible simultaneously with the fragmented/partial protocol without the possibility for false positive, e.g.
TEST_P(ProxyProtocolTest, TinyPartialV1ReadWithAllowNoProxyProtocolThisFails) {
envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
proto_config.set_allow_requests_without_proxy_protocol(true);
connect(true, &proto_config);
write("P"); // matches the beginning of v1 proxy protocol
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
write("BOGUS");
expectData("PBOGUS"); // we will consume `P` and upstream gets `BOGUS` not `PBOGUS`
disconnect();
}
this is why in the initial implementation I required that the initial read from recv
have enough bytes available on the initial MSG_PEEK
to make the determination on the v1/v2 proxy header.
We can leave the PR as it is (or perhaps preferably throw a hard error if we consumed some bytes and then realize we had a false positive?) with the understanding that there could be false positives on identifying proxy protocol, additionally with the understanding that this is quite rare and will not likely happen in practice (except perhaps for single byte reads of P
and HTTP POST/PUT request?). In exchange, we get support for fragmented/partial reads. Thoughts?
Also willing to go back to my original version (before 6a0d49b which addresses the initial concern with partial reads) which addresses the potential false positive here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following. How would the filter consume the P
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the test above, we do recv
with MSG_PEEK
and just get P
here.
Then before we continue the loop, we read that byte for real (no MSG_PEEK
) here. Thus when we do our second recv
and we get BOGUS
we have already consumed P
. We correctly identify that the request is not proxy protocol, but the request forwarded upstream has a missing byte(s) (in this case, just P
).
The only way I see to avoid false positives identifying the header and always send the correct response up the filter chain is to require that the initial MSG_PEEK
has enough bytes to detect v1/v2 header (at most 16, which seems reasonable for this opt-in only codepath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see.
I think you need to fix this case. You may have to restructure the code such that no bytes are read (without MSG_PEEK) until we've decided 100% whether this is a proxy header or not.
I think it's probably good enough to look for either PROXY_PROTO_V1_SIGNATURE
or PROXY_PROTO_V2_SIGNATURE
. If the connections starts with either of those, but then doesn't have a valid full proxy protocol header, abort the connection as we currently do. If the connection starts with bytes that match neither of those signatures, and the new option is enabled, don't read()
any bytes and continue the filter chain. Will that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -434,6 +445,13 @@ ReadOrParseState Filter::readProxyHeader(Network::IoHandle& io_handle) { | |||
if (nread < 1) { | |||
ENVOY_LOG(debug, "failed to read proxy protocol (no bytes read)"); | |||
return ReadOrParseState::Error; | |||
} else if (nread < PROXY_PROTO_V2_HEADER_LEN && | |||
config_.get()->allowRequestsWithoutProxyProtocol()) { | |||
if (nread < PROXY_PROTO_V1_SIGNATURE_LEN || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is rare case, there still have the chance of receiving the partial v1 signature, like just a 'P' char, the left of the signature received by the second recv
call. But this check will just skip the left of signature checking.
we could return SkipFilterError
at https://github.com/envoyproxy/envoy/pull/18951/files#diff-a35fc10b1ce97239d02ac1791aac05069eaf253d8efc77cefe93749f901e36abL446
Then it will got all the data, and compared the signature of v1 and v2.
But if we only receive very short data from the client (short than both v1 and v2 signature), then I feel we only can waiting for a timeout of listener filter. But that is bad for the usecase of issue.
Another idea is if the nread
is less than v1 and v2 signature, then we compare part of signature, like if the first byte is X', then compare to the first byte of v1 signature, it is
P`. Also compare the first byte of v2 signature, then we know it can't be the proxy protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soulxu can you update the link from
we could return SkipFilterError at https://github.com/envoyproxy/envoy/pull/18951/files#diff-a35fc10b1ce97239d02ac1791aac05069eaf253d8efc77cefe93749f901e36abL446
Then it will got all the data, and compared the signature of v1 and v2.
to a link to main / a commit and where you're proposing this change? I think with the recent commits to this PR that the link provided is dated, I'm not sure where you're proposing we return the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also related #18951 (comment)
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
I like the current version better, due to simplicity. In nearly all cases, all the bytes for comparision will arrive in the first packet, and we're only comparing roughly two machine words of data (on 64-bit); I think the performance difference will be unmeasurable. I think the simplicity out-weights the small amount of duplication (of cpu cycles and code) in this case. |
Agree with this version is more simple. @kdorosh thanks for your patience and trying out two versions, currently code is LGTM now |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh; I wrote all this yesterday and forgot to hit "submit review". Sorry for the delay.
/wait
// For more information on the security implications of this feature, see | ||
// https://www.haproxy.org/download/2.1/doc/proxy-protocol.txt | ||
// | ||
// While incredibly rare, requests of 12 or fewer bytes that match the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove "While incredibly rare" from the docs.
Also, I think this paragraph should either have attention::
or note::
header above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
@@ -45,6 +45,7 @@ New Features | |||
* dns_resolver: added :ref:`include_unroutable_families<envoy_v3_api_field_extensions.network.dns_resolver.apple.v3.AppleDnsResolverConfig.include_unroutable_families>` to the Apple DNS resolver. | |||
* ext_proc: added support for per-route :ref:`grpc_service <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExtProcOverrides.grpc_service>`. | |||
* on_demand: :ref:`OnDemand <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.OnDemand>` got extended to hold configuration for on-demand cluster discovery. A similar message for :ref:`per-route configuration <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.PerRouteConfig>` is also added. | |||
* proxy protocol: added support for allowing requests without proxy protocol on the listener from trusted downstreams as an opt-in flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to the new config setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
@@ -78,6 +83,8 @@ Network::FilterStatus Filter::onData(Network::ListenerFilterBuffer& buffer) { | |||
return Network::FilterStatus::StopIteration; | |||
} else if (read_state == ReadOrParseState::TryAgainLater) { | |||
return Network::FilterStatus::StopIteration; | |||
} else if (read_state == ReadOrParseState::SkipFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this into a switch/case on read_state
? Then the compiler can warn if not all cases are handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
if (!matchv2 && !matchv1) { | ||
// The bytes we have seen so far do not match v1 or v2 proxy protocol, so we can safely | ||
// short-circuit | ||
ENVOY_LOG(debug, "request does not use v1 or v2 proxy protocol, forwarding as is"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be at trace level because it is expected to happen in this configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
private: | ||
absl::flat_hash_map<uint8_t, KeyValuePair> tlv_types_; | ||
const bool allow_requests_without_proxy_protocol_{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need the {}
initializer at the end because it is always initialized in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
std::string msg = "data"; | ||
EXPECT_GT(PROXY_PROTO_V1_SIGNATURE_LEN, | ||
msg.length()); // Ensure we attempt parsing byte by byte using `search_index_` | ||
EXPECT_GT(PROXY_PROTO_V2_SIGNATURE_LEN, msg.length()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
EXPECT_GT(PROXY_PROTO_V2_SIGNATURE_LEN, msg.length()); | ||
|
||
write(msg); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
write(msg); | ||
|
||
expectData(msg); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
connect(true, &proto_config); | ||
|
||
std::string msg = "more data more data more data"; | ||
EXPECT_GT(msg.length(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_GT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
@@ -526,6 +608,19 @@ TEST_P(ProxyProtocolTest, V2ShortV4) { | |||
expectProxyProtoError(); | |||
} | |||
|
|||
TEST_P(ProxyProtocolTest, V2ShortV4WithAllowNoProxyProtocol) { | |||
// An ipv4/tcp connection that has incorrect addr-len encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An ipv4/tcp PROXY header that....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 392124e
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
@ggreenway no problem at all, thanks again for the feedback and detailed review! |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one final nit.
@adisuissa can you review the API?
} else if (read_state == ReadOrParseState::SkipFilter) { | ||
case ReadOrParseState::SkipFilter: | ||
return Network::FilterStatus::Continue; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the default case. Then the compiler will warn/error if there are missing enum cases. I think it's possible that you won't need the return
outside the switch either, if all cases are handled and all of them have a return
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 5c50e23
unfortunately the compiler isn't smart enough to tell that every case is implemented and returns; thus we need the ghost "default" case at the end of the function after the switch statement
source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc: In member function 'virtual Envoy::Network::FilterStatus Envoy::Extensions::ListenerFilters::ProxyProtocol::Filter::onData(Envoy::Network::ListenerFilterBuffer&)':
source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc:93:1: error: control reaches end of non-void function [-Werror=return-type]
93 | }
| ^
I removed the default case regardless and confirmed that compiler will fail if any case is not implemented
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
/retest |
Retrying Azure Pipelines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm api
@adisuissa ci is passing; I think we need a final approval from you here since you requested changes before? Thanks! |
…voyproxy#18951) Allows users to opt-in to functionality to auto-detect proxy protocol if present, and skip the filter if it's not present. Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh kevin.dorosh@solo.io
Commit Message: Auto-detect proxy protocol
Additional Description: Allows users to opt-in to functionality to auto-detect proxy protocol if present, and skip the filter if it's not present.
Risk Level: Low, opt-in new feature only hits new codepath if configured
Testing: Added tests for new codepath that cover both passthrough and fragmented proxy protocol scenarios
Docs Changes: N/A, outside of auto-generated API docs
Fixes: #18888